Check bounding boxes and trees before starting neuron segmentation #8678
Conversation
📝 WalkthroughWalkthroughThe changes introduce validation logic to prevent starting split/merger evaluation jobs when the ground truth is unusable, such as missing or multiple bounding boxes, missing skeleton annotations, or empty skeleton trees. Explanatory UI text and localized error messages are added. Supporting utility functions and selector updates are also included. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (1)
120-122: CheckNodeMapAPI & consider clearer naming
tree.nodes.size()assumesNodeMapexposes asize()method. In several places of the code base I have seensizeused as a property. Please double-check the actual API – a silentundefined is not a functionat runtime would abort every job submission.- The helper returns
truewhen at least one tree is empty. Naming ithasEmptyTreesis therefore slightly ambiguous – something likecontainsEmptyTree/anyTreeEmptycommunicates intent more clearly.No blocker, but please verify (1) and, if you agree, adjust the name or add a short JSDoc.
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx (1)
640-652: Small wording / accessibility nitInside the explanatory
<div>the bullet list is wrapped in another<div>without a semantic heading; consider replacing the outer<div>by a<p>or<Fragment>and adding a<strong>heading so screen readers don’t flatten text & list into one blob.Pure polish – feel free to ignore if UI copy is still in flux.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts(1 hunks)frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (1)
frontend/javascripts/viewer/model/types/tree_types.ts (2)
TreeMap(94-94)Tree(61-75)
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx (3)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (1)
getUserBoundingBoxesFromState(102-105)frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (1)
hasEmptyTrees(120-122)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx (1)
1122-1131: Guard clauses look good, just ensure consistentsize()usageThis block now relies on
trees.size()and onhasEmptyTrees(which internally callssize()as well). IfNodeMapever switches back to asizeproperty, both checks would fail the same way. Once you have verified the API in the accessor file, no further action is required here.
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/viewer/controller/scene_controller.ts (2)
338-345: Remove unused loop variable & consider deleting obsolete map entries
for (const [tracingId, _boundingBox] of Object.entries(this.taskCubeByTracingId)) { ... }
The second tuple element (_boundingBox) is never used – it can be omitted to avoid the ESLintno-unused-varswarning.Additionally, instead of setting
this.taskCubeByTracingId[tracingId] = null;, you coulddeletethe key altogether to keep the map clean and avoid accumulating stale keys over time:-for (const tracingId of Object.keys(this.taskCubeByTracingId)) { - const taskCube = this.taskCubeByTracingId[tracingId]; - ... - this.taskCubeByTracingId[tracingId] = null; -} +for (const tracingId of Object.keys(this.taskCubeByTracingId)) { + const taskCube = this.taskCubeByTracingId[tracingId]; + ... + delete this.taskCubeByTracingId[tracingId]; +}This keeps memory usage deterministic and makes subsequent
Object.entriesiterations cheaper.
347-367: Minor cleanup: avoid redundant state look-ups and unused variableWithin the “add new entries” loop:
Store.getState()is called for every iteration only to testtask == null. Capture the result once before the loop for efficiency (even if it’s run rarely).let taskCube = this.taskCubeByTracingId[tracingId];is assigned but never read before being overwritten. Remove the line to reduce noise.-const { viewMode } = Store.getState().temporaryConfiguration; +const { viewMode } = state.temporaryConfiguration; ... -let taskCube = this.taskCubeByTracingId[tracingId];These micro-cleanups improve readability without altering behaviour.
frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (1)
94-99: Guard against undefinedboundingBoxvalues
Object.fromEntries(layers.map((l) => [l.tracingId, l.boundingBox]))will include entries whoseboundingBoxmight beundefined.
Down-stream code partly copes withnull, butundefinedslips through theboundingBox == nullcheck (undefined == null➜true) and causes an unnecessary key that is immediately discarded inupdateTaskBoundingBoxes.Filter here to return only layers with a concrete bounding box:
-return Object.fromEntries(layers.map((l) => [l.tracingId, l.boundingBox])); +return Object.fromEntries( + layers + .filter((l) => l.boundingBox != null) + .map((l) => [l.tracingId, l.boundingBox]), +);This keeps the selector purer and reduces needless churn in the scene controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/viewer/controller/scene_controller.ts(2 hunks)frontend/javascripts/viewer/model/accessors/tracing_accessor.ts(1 hunks)frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/messages.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/viewer/controller/scene_controller.ts (1)
frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (1)
getTaskBoundingBoxes(101-101)
frontend/javascripts/viewer/model/accessors/tracing_accessor.ts (2)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(559-579)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (1)
annotation(167-219)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
|
@philippotto I read the slack conversation again and understood that the checks I should be doing are independant of the selcted bounding box. Thus I applied this, in addition to the things we discussed this morning. |
URL of deployed dev instance (used for testing):
Steps to test:
enable long-running jobs
create an annotation and open the
AI jobsmodal. select neuron segmentation. enableEvaluation settings. Either of the following conditions should yield a corresponding error toasts and prevent you from starting the job:all this should be independent of the selected bounding box.
open the annotation of a finished task. if a bounding box appears in the bounding box tab, this bounding box should be selectable in the bounding box dropdown of the modal. after selecting it (given that there are skeletons with nodes), you should be able to start the job.
the scene controller was adjusted in the place where the task bounding boxes are updated. thus you should create some tasks with different bounding boxes and check after clicking
Finish and get next taskthe lime green task bounding box is updated within the view port.context
questions/feedback: https://scm.slack.com/archives/C5AKLAV0B/p1749201952606739
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)